Fix fialed to watch context canceled error when streaming logs
#2681
Conversation
tekton-robot
added
the
release-note
tekton-robot
added
the
size/M
pratap0007
force-pushed
the
fix-pr-log-err
branch
2 times, most recently
from
40e0002 to
5a77d7b
Compare
|
/retest |
|
cc: @divyansh42 |
divyansh42
left a comment
divyansh42
left a comment
There was a problem hiding this comment.
Thanks for fixing this.
Changes looks good to me, I just added a minor comment.
pkg/pipelinerun/tracker.go
Outdated
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/apimachinery/pkg/fields" | ||
| "k8s.io/client-go/tools/cache" | ||
| k8scache "k8s.io/client-go/tools/cache" |
There was a problem hiding this comment.
[Minor] Any specific reason for this rename?
I can see in other files we are using cache so this will be a bit unconsistent.
When streaming pipelineRun or taskRun logs, closing the stop channel to terminate the informer was causing "Failed to watch" error messages to be logged with "context canceled". This patch add custom watchErrorHandler function that filter out context.Canceled errors while passing other errors to the default handler. This prevents the error log messages when the CLI intentionally stops watching after a run completes. Signed-off-by: Shiv Verma <[email protected]>
pratap0007
force-pushed
the
fix-pr-log-err
branch
from
5a77d7b to
6d5bb35
Compare
|
/lgtm |
|
/retest |
|
cc: @vdemeester |
vdemeester
left a comment
vdemeester
left a comment
There was a problem hiding this comment.
Great fix! This addresses a real UX issue where users were seeing confusing error messages during normal operation.
Summary
✅ Functionality: Correctly filters context.Canceled errors using errors.Is()
✅ Test Coverage: Good tests for both direct and wrapped errors
✅ Consistency: Applied consistently across both packages
✅ CI/CD: All checks passing
Minor Issues
💡 Optional: Small code duplication (see inline comments)
The typo is just in the PR title/description, not in the code itself. You can fix it by editing the PR title and the release notes section.
Overall, this is good to merge! I've left a few inline suggestions below, but they're all optional improvements.
Approved!
|
|
||
| // Set a custom watch error handler that ignores context.Canceled errors | ||
| // to prevent "Failed to watch" log messages when the informer is stopped intentionally | ||
| _ = informer.SetWatchErrorHandlerWithContext(watchErrorHandler) |
There was a problem hiding this comment.
Unused return value
The error return from SetWatchErrorHandlerWithContext is ignored here. This is probably fine since it's a configuration step during initialization, but if you wanted to be more defensive, you could log any errors:
if err := informer.SetWatchErrorHandlerWithContext(watchErrorHandler); err != nil {
// Log or handle the error
}That said, the current approach is acceptable for simplicity.
| // watchErrorHandler is a custom watch error handler that filters out context.Canceled errors | ||
| // to prevent "Failed to watch" log messages when the informer is stopped intentionally. | ||
| // Other errors are passed to the default handler. | ||
| func watchErrorHandler(ctx context.Context, r *cache.Reflector, err error) { |
There was a problem hiding this comment.
💡 Code Duplication (Optional)
This watchErrorHandler function is duplicated in both pkg/pipelinerun/tracker.go and pkg/pods/pod.go.
Options:
- Keep as-is (acceptable) - The function is simple (4 lines) and keeping it local to each package avoids introducing dependencies
- Extract to shared utility - Could move to something like
pkg/utils/watch/error_handler.goif you're concerned about maintainability
Given the simplicity, the current approach is perfectly reasonable. This is more of an FYI than a required change.
| } | ||
| } | ||
|
|
||
| func TestTracker_watchErrorHandler(t *testing.T) { |
There was a problem hiding this comment.
✅ Excellent test coverage!
Love that you're testing both direct context.Canceled and wrapped versions with errors.Join(). This ensures the errors.Is() logic works correctly in all cases.
The approach of passing nil for the reflector is clever since the handler should filter these errors before reaching the code that would use it.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: divyansh42, vdemeester The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
tekton-robot
added
the
approved
4 participants
When streaming pipelineRun or taskRun logs, closing the stop channel to terminate the informer was causing "Failed to watch" error messages to be logged with "context canceled". This was noisy and confusing
for users even though it was expected behavior during intentional
shutdown.
This patch add custom watchErrorHandler function that filter out context.Canceled errors while passing other errors to the default handler. This prevents the error log messages when the CLI intentionally stops watching after a run completes.
Changes
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
make checkmake generatedSee the contribution guide
for more details.
Release Notes